Repository navigation
Preserve one-shot operation iterators during patch validation - #234
Apeksha9389 wants to merge 3 commits into
Conversation
|
coverage: 100.0%. remained the same — Apeksha9389:fix/preserve-operation-iterators into stefankoegl:master |
stefankoegl
left a comment
stefankoegl
left a comment
There was a problem hiding this comment.
Thanks for the fix! Materializing one-shot input before validation is the right approach, and plain iterators and generators now work correctly. The test suite passes on this head.
Before merging, I'd ask for the change suggested on jsonpatch.py:519: check for Sequence instead of Iterator, so that one-shot iterables without __next__ are covered too. Inline comments have the details. Two more points that aren't on changed lines:
- Docs: the
apply_patchdocstring still says:type patch: list or str, and theJsonPatchclass docstring anddoc/tutorial.rstonly describe building a patch from a list. If iterators and generators are now supported, it would help to say so. - Optional, not blocking:
__init__builds every operation object only to validate it, then throws them away, and_opsrebuilds them on everyapply,__eq__and__hash__. Since iterator input is now copied anyway, the validated operations could be stored and reused. This behavior predates this PR, so it's fine to leave it for a follow-up.
Generated by Claude Code
stefankoegl
left a comment
stefankoegl
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround! 504f40e addresses all the earlier comments:
Sequencecheck instead ofIterator✅, with a test for a one-shot iterable that has no__next__✅- the
assertIsidentity test is replaced by content and application checks for lists and tuples ✅ - named subtests ✅
apply_patchandJsonPatchdocstrings anddoc/tutorial.rstupdated ✅
Still blocking: CI is red on every Python version. That isn't a flaw in this commit itself. #236 (merged to master yesterday) restructured the same constructor, and the merged code skips validation for one-shot inputs. Details and a tested fix are inline. Once master is merged in and the copy moved below the type checks, this looks ready to me.
Optional nit: the collections.abc import is now only rewrapped in parentheses with the same names, so it could go back to its original single line to keep the diff minimal.
Generated by Claude Code
|
|
||
| def __init__(self, patch, pointer_cls=JsonPointer): | ||
| self.patch = patch | ||
| self.patch = patch if isinstance(patch, Sequence) else list(patch) |
There was a problem hiding this comment.
This line is right on its own, but it now clashes with #236, which merged into master after this branch was created. CI tests the merge with master, and that's why every build job is red.
On master, __init__ now checks the input type after self.patch = patch and then validates by iterating the original patch:
self.patch = patch
...
if isinstance(patch, (str, bytes, Mapping)):
raise InvalidJsonPatch(...)
try:
patch_iter = iter(patch)
except TypeError:
raise InvalidJsonPatch(...)
for op in patch_iter:
...Git merges the two without a conflict, but the result has two problems:
list(patch)runs before the type checks, so non-iterable input likeJsonPatch.from_string('1')raisesTypeErrorinstead ofInvalidJsonPatch. That accounts for the 18 errors intest_serialized_patch_rejects_non_array_documents.- More seriously,
list(patch)has already used up a one-shot input, soiter(patch)yields nothing and no operation gets validated.test_invalid_operation_iterator_is_rejectedcatches this (good test!). An invalid iterator is accepted, and the error only appears later inapply().
Moving the copy after the type checks and validating self.patch fixes both:
def __init__(self, patch, pointer_cls=JsonPointer):
self.pointer_cls = pointer_cls
if isinstance(patch, (str, bytes, Mapping)):
raise InvalidJsonPatch("Document is expected to be sequence of operations")
try:
patch_iter = iter(patch)
except TypeError:
raise InvalidJsonPatch("Document is expected to be sequence of operations")
# One-shot iterables are materialized, so that validating them does
# not leave nothing to apply
self.patch = patch if isinstance(patch, Sequence) else list(patch_iter)
# Verify that the structure of the patch document
# ...
for op in self.patch:I tried this on top of this branch merged with current master (e1d055d). All 227 tests and the module doctests pass. Could you merge master into the branch and apply it?
Generated by Claude Code
There was a problem hiding this comment.
Implemented in 30311c1 after merging master. Materialization now runs inside the guarded iterable validation, after rejecting strings, bytes and mappings, and operation validation iterates self.patch. All 227 local tests and module doctests pass. CI is now green on Python 3.10 through 3.14, including coverage at 100%.
JsonPatch(iter(operations)).apply(document)currently applies no operations: construction validates and exhausts the iterator before application can read it. Empty iterators also produce a truthy patch, and serializing an iterator-backed patch fails.Materialize one-shot iterators before validation. Existing list and tuple inputs retain their container behavior. Regression tests cover list iterators and generators, repeated application and serialization, empty iterators, invalid operations, and preservation of the original list container.
Validation on Python 3.12.14: 217 unit tests and 20 Hypothesis property tests pass. The iterator and empty-iterator regressions produce three failures without the fix. Ruff syntax/undefined-name checks and
git diff --checkpass. Windows and the other Python versions were not run locally.AI disclosure: This patch, tests, and description were prepared with OpenAI Codex assistance. The reported checks were executed locally.